feat: Hanko passkeys support #14741
feat: Hanko passkeys support #14741
Conversation
|
@merlindru is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
app-store
|
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link. |
.env.example
Outdated
| @@ -116,15 +116,31 @@ NEXT_PUBLIC_FRESHCHAT_HOST= | |||
| # @see https://support.google.com/cloud/answer/6158849#public-and-internal&zippy=%2Cpublic-and-internal-applications | |||
| GOOGLE_LOGIN_ENABLED=false | |||
|
|
|||
| # - GOOGLE CALENDAR/MEET/LOGIN | |||
| # Hanko Passkeys Config | |||
There was a problem hiding this comment.
lets mention this lower in the file
There was a problem hiding this comment.
yeah was unsure about that. wanted to place it below google auth stuff since i did that everywhere else, but agreed, for the env file its a little too high up
|
We'd really like to use the passkey icon (not part of lucide, it's an SVG) however the components that made use of it (Button, EmptyPage, ...) have all been changed to only accept a lucide icon name (string) Is there any way we still could use a custom component as an icon for these? |
dosubot
bot
added
authentication
Graphite Automations"Add foundation team as reviewer" took an action on this PR • (04/25/24)1 reviewer was added to this PR based on Keith Williams's automation. "Add consumer team as reviewer" took an action on this PR • (04/25/24)1 reviewer was added to this PR based on Keith Williams's automation. |
|
While PASSKEY_LOGIN_ENABLED hides all passkey related UI on the login page, the settings page as well as the API routes to register and remove passkeys stay enabled These aren't strictly related to logging in with Passkeys; do you feel like the env var should disable everything related to passkeys or just the ability to login? |
| @@ -62,6 +63,7 @@ | |||
| "@stripe/react-stripe-js": "^1.10.0", | |||
| "@stripe/stripe-js": "^1.35.0", | |||
| "@tanstack/react-query": "^5.17.15", | |||
| "@teamhanko/passkeys-next-auth-provider": "^0.2.7", | |||
There was a problem hiding this comment.
I wish Node had a better way to make optional dependencies 🤷♂️
There was a problem hiding this comment.
I'm not sure since when npm has this, but there is optionalDependencies
https://docs.npmjs.com/cli/v10/configuring-npm/package-json#optionaldependencies
Was supported in yarn v1 so I assume v2+ has it too
There was a problem hiding this comment.
Opted not to do this for now because optionalDependencies isn't in use for any other packages atm. Also I'm unsure if a build without installing optional deps would be successful, especially because of import behavior, haven't tested it
Do you want me to test this? What other not-strictly-needed dependencies are there that could be moved to optionalDependencies (at least in theory)?
|
|
||
| export default async function handler(req: NextApiRequest, res: NextApiResponse) { | ||
| if (!tenantId || !apiKey) { | ||
| return res.status(503).json({ message: "Passkey API not configured" }); |
There was a problem hiding this comment.
Is 503 the best status code to use here?
There was a problem hiding this comment.
Other routes use 403: https://github.com/calcom/cal.com/blob/main/apps/web/pages/api/auth/signup.ts#L23
However, MDN says
[...] 503 Service Unavailable server error response code indicates that the server is not ready to handle the request.
Common causes are a server that is down for maintenance or that is overloaded. This response should be used for temporary conditions [...]
While in my mind 403 has always been more for insufficient permissions etc
What do you think? Maybe 403 for consistency's sake?
There was a problem hiding this comment.
I'd prefer 403 Forbidden, 503 implies a server issue but most of the time this'll be a client issue (don't access the route)
There was a problem hiding this comment.
Don't think a 403 is correct here though because that leads the client to believe they are lacking permissions when in reality the functionality isn't supported at all due to a lack of server configuration. Would consider 501 Not Implemented or 405 Method Not Allowed instead.
There was a problem hiding this comment.
There was a problem hiding this comment.
If everyone is fine with it, will go with 501 - seems like the most appropriate IMO
There was a problem hiding this comment.
While PASSKEY_LOGIN_ENABLED hides all passkey related UI on the login page, the settings page as well as the API routes to register and remove passkeys stay enabled
These aren't strictly related to logging in with Passkeys; do you feel like the env var should disable everything related to passkeys or just the ability to login?
In line with current practises it's not needed to fully "unregister" routes
We'd really like to use the passkey icon (not part of lucide, it's an SVG)
Feel free to modify the button component to allow StartIcon's that aren't strings, ideally in a way it doesn't accept lucide icons (strings should be used instead).
| const IS_PASSKEY_LOGIN_ENABLED = !!( | ||
| process.env.PASSKEY_LOGIN_ENABLED === "true" && | ||
| process.env.NEXT_PUBLIC_HANKO_PASSKEYS_TENANT_ID && | ||
| process.env.HANKO_PASSKEYS_API_KEY | ||
| ); | ||
|
|
There was a problem hiding this comment.
I think we already have this in constants - lets put it from there
There was a problem hiding this comment.
We do, but the GOOGLE variables are also duplicated in next-auth-options, so I did the same to stay consistent. Do you know if there's any reason for this?
Otherwise we could import both from constants.ts.
(I don't think the google constants differ from the ones in next-aut-options.ts in any way)
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is an install script?Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts. Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
|
Thank you @PeerRich and reviewers! If this is it, then I'll make one commit to change the |
Base branch was modified
Passkeys requires "use client".
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 New Page AddedThe following page was added to the bundle from the code in this PR:
One Page Changed SizeThe following page changed size from the code in this PR compared to its base branch:
DetailsOnly the gzipped size is provided here based on an expert tip. First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If Any third party scripts you have added directly to your app using the The "Budget %" column shows what percentage of your performance budget the First Load total takes up. For example, if your budget was 100kb, and a given page's first load size was 10kb, it would be 10% of your budget. You can also see how much this has increased or decreased compared to the base branch of your PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. If you see "+/- <0.01%" it means that there was a change in bundle size, but it is a trivial enough amount that it can be ignored. |
|
@merlindru still has a consistently failing e2e test; could you have a looksy? |
Merge queue setting changed
|
@emrysal Sorry, maybe I'm being dumb, but which e2e test is failing/how can I run it? With Also thanks for staying on top of this :) Much appreciated! |
Note: This previously was #13127 but has since been reworked and simplified
What does this PR do?
Fixes #13342
Fixes CAL-2984
This PR adds passkey support for logging in through Hanko Passkeys.
Type of change
How should this be tested?
set it to
HANKO_PASSKEYS_API_KEY="<api key>"(quotes important)
NEXT_PUBLIC_HANKO_PASSKEYS_TENANT_ID=<tenant id>Mandatory Tasks
next-auth-options.ts
This file now includes a PasskeyProvider.
For its
authorizefunction, I've collected all of the important checks I could find in the email+password provider (a.k.a. CredentialsProvider), such as rate-limiting, user.locked, belongsToActiveTeams, ...) and included them in the PasskeyProvider.For some of these, the fields/relations returned by
UserRepository.findByIdwasn't enough. Unfortunately.findTeamsByUserIddoesn't return enough info eitherI didn't want to touch any of the existing queries, so I've added
UserRepository.findByIdAndIncludeProfilesAndPassword...which is the exact same as the email version of this function (
findByEmailAndInclude...). It just looks the user up by their ID instead.Checklist
UI
Note about images
These are the same images from the old PR. There's one minor change since then: since icon components aren't supported for
Button,EmptyPage, ... anymore, I used Lucide'skey-roundiconThis is what key-round looks like:
Versus the passkey icon:
Adds passkey button to login page:
Adds "Passkeys" settings section to sidebar:
Adds passkeys settings page
Empty:
Clicking on the "add" button and "sign in with a passkey" buttons opens a browser dialog. This looks different for every browser, phone, etc.
For example:
With passkeys added:
Dropdown stolen straight from API settings page:
